Support config matrix for SLT tests by comphead · Pull Request #24493 · apache/datafusion

comphead · 2026-08-19T17:02:05Z

Which issue does this PR close?

Closes Support config matrix for slt tests #24390 .

Rationale for this change

Config knobs like datafusion.execution.parquet.coerce_int96 and coerce_int96_tz should produce the same observable result across values. Today that requires a separate .slt per combination. This PR
sweeps a cartesian product of config values from a single file.

What changes are included in this PR?

New # configMatrix: <key>=<v1>,<v2>[,...] directive. Repeat to nest dimensions; repeated keys merge value lists.
New datafusion/sqllogictest/src/config_matrix.rs - parser + expansion, exports parse_config_matrix_from_file, matrix_tag, ConfigMatrixCombination.
bin/sqllogictests.rs: run_test_file dispatches once per combo via run_test_file_once, which applies values through config_mut().options_mut().set(k, v) on a fresh SessionContext. Errors get a
single [configMatrix: k=v, ...] suffix.
New test_files/parquet_int96_matrix.slt - 2×2 sweep over coerce_int96 and coerce_int96_tz.
Cookbook section added to datafusion/sqllogictest/README.md.

Are these changes tested?

17 unit tests in config_matrix::tests cover parsing, dedup, merge, cartesian expansion, and error paths.
parquet_int96_matrix.slt exercises the runner end-to-end.

Are there any user-facing changes?

Additive only. Files without a directive run byte-for-byte as before. Matrix-scoped failures include [configMatrix: ...] in the banner.

codecov-commenter · 2026-08-19T19:12:40Z

Codecov Report

❌ Patch coverage is 88.99083% with 60 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.46%. Comparing base (26b40dd) to head (1ad33fc).
⚠️ Report is 29 commits behind head on main.

Files with missing lines	Patch %	Lines
datafusion/sqllogictest/bin/sqllogictests.rs	58.20%	49 Missing and 7 partials ⚠️
datafusion/sqllogictest/src/config_matrix.rs	98.83%	3 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #24493      +/-   ##
==========================================
+ Coverage   81.43%   81.46%   +0.02%     
==========================================
  Files        1118     1120       +2     
  Lines      399414   400879    +1465     
  Branches   399414   400879    +1465     
==========================================
+ Hits       325278   326577    +1299     
- Misses      55145    55224      +79     
- Partials    18991    19078      +87

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

kosiew

@comphead,

Thanks for working on this. The config matrix support looks useful and the overall implementation is nicely scoped.

I found one issue that I think needs to be addressed before merging. The Substrait round-trip path currently bypasses the config matrix handling, so a matrix-bearing SLT file can silently run only once in that mode. I also left one small documentation suggestion about where the matrix tag appears in failure output.

kosiew · 2026-08-20T05:19:23Z

                    options.substrait_round_trip,
                ) {
                    (_, _, true) => {
                        run_test_file_substrait_round_trip(


It looks like --substrait-round-trip still dispatches directly to run_test_file_substrait_round_trip, so this path never parses or applies configMatrix combinations.

That means an .slt file with a matrix can silently run only once in this supported mode, even though the directive suggests the file will be exercised across all combinations. Could we either route this path through the same per-combination setup, or explicitly reject configMatrix when Substrait round-trip mode is used?

It would also be good to add a regression test that verifies each matrix combination is actually executed.

kosiew · 2026-08-20T05:20:49Z

+- Repeat the directive to nest keys. Values are the cartesian product.
+- Whitespace-trimmed and deduped; repeated keys merge value lists.
+- Unknown key or invalid value fails fast, naming the file, key, and value.
+- Test failures include `[configMatrix: k=v, ...]` in the `N errors in file …` banner.


Small documentation nit: this says the matrix tag is included in the N errors in file ... banner, but run_test_file_once currently appends the tag to the fully formatted error after the per-record errors.

Could we either move the tag into that banner or adjust the wording here to describe where it actually appears?

comphead · 2026-08-24T19:10:22Z

Thanks @kosiew for the review, addressed the feedback please have another look

kosiew

@comphead,

Thanks for working through the earlier feedback. The Substrait round-trip path now expands and applies each config matrix combination, and the README wording around the failure banner looks fixed.

I found two remaining issues that I think should be addressed before merging. The CI smoke filter points to a test file that does not exist, so the requested matrix regression coverage is still missing. There is also a compatibility regression in the new Clap handling for SLT_TIMING_DEBUG_SLOW_FILES, where previously accepted values such as 1 now cause the test binary to abort.

I left inline comments with the details and suggested fixes.

kosiew · 2026-08-25T11:52:06Z

                    "--",
                    "--substrait-round-trip",
                    "limit.slt",
+                    "configmatrix_show_smoke.slt",


I think the regression coverage is still missing here. configmatrix_show_smoke.slt is passed as a CI filter, but there is no tracked file with that name under datafusion/sqllogictest/test_files.

Because unmatched file filters are silently ignored, this step still only runs limit.slt, so no matrix directive reaches the Substrait round-trip path. That means the earlier request for a regression test that proves every matrix combination is executed is not actually covered yet.

Could we either add the missing smoke .slt, ideally with alternatives where skipping either combination would produce an observable failure, or point this filter at an existing matrix-bearing test file?

Thanks @kosiew this is a good catch! it was a smoke test and shouldn't be bleeding into the code

Addressed in 49ead27

kosiew · 2026-08-25T11:52:06Z

+    #[clap(
+        long,
+        env = "SLT_TIMING_DEBUG_SLOW_FILES",
+        default_value_t = false,


This looks like a compatibility regression in the environment variable handling. The previous SLT_TIMING_DEBUG_SLOW_FILES parser accepted 1, true, yes, and on case-insensitively, while other values were treated as disabled. With Clap's bool parser, only true and false are accepted, and a previously valid value such as 1 now aborts the entire test binary with a usage error.

I verified this with SLT_TIMING_DEBUG_SLOW_FILES=1 cargo test -p datafusion-sqllogictest --test sqllogictests --features substrait -- --list --format terse, which exits 2 with invalid value '1'.

Could we preserve the previous parsing semantics, for example with a custom Clap value parser, or keep the existing environment-only parser and expose the CLI flag separately?

comphead · 2026-08-25T18:00:01Z

Thanks @kosiew for catching issues, addressed them and made the matrix be less intrusive to the runner

kosiew

@comphead,

Thanks for continuing to work through the configMatrix support. The successful Substrait dispatch path and the legacy SLT_TIMING_DEBUG_SLOW_FILES parsing both look fixed now.

I still found two issues that I think should be addressed before merging. The first is around runtime configuration keys, and the second is that a failure in one matrix configuration prevents the remaining combinations from running.

Validation I ran:

cargo test -p datafusion-sqllogictest config_matrix --lib passes with 22 tests.
A temporary two-value matrix under --substrait-round-trip passes and confirms the normal successful dispatch path.
A temporary matrix using datafusion.runtime.memory_limit=100M fails with Config value "runtime" not found on ConfigOptions.

Thanks again for the updates here.

kosiew · 2026-08-26T10:12:59Z

+    /// `origin` is a display label for error messages (typically the test
+    /// file path).
+    ///
+    /// Note this sets config options directly rather than going through


I think this still leaves an important gap in what configMatrix can configure. The comment notes that these overrides are applied directly to ConfigOptions instead of going through the SessionContext SET path, but the README presents configMatrix more generally as a way to sweep configuration values.

For example, a matrix containing datafusion.runtime.memory_limit=100M currently fails with Config value "runtime" not found on ConfigOptions, even though SET datafusion.runtime.memory_limit = '100M' is supported.

Could we route matrix overrides through the same runtime-config and config-dependent UDF refresh path used by SessionContext::set_variable, or extract a shared helper for both paths? It would also be good to add coverage for a runtime setting and any config-dependent UDF behavior. If matrices are intentionally limited to ConfigOptions, then I think that narrower scope should be explicitly enforced and documented.

kosiew · 2026-08-26T10:12:59Z

+                .await;
+        pb.finish_and_clear();
+
+        test_configuration.attribute_failure(result)?;


It looks like a failure in the first Substrait matrix configuration is still propagated with ?, so the remaining matrix combinations never run. The default runner appears to have the same behavior around line 562.

That means a file does not actually run once for every combination when an earlier combination fails, and it also makes it hard to see all configuration-specific failures in one run.

Could we keep executing the remaining configurations and aggregate the failures with enough context to identify the configuration that produced each one? I would also like to see a tracked matrix SLT or integration test where skipping a later combination is observable, so this behavior is covered for the Substrait path as well.

Thanks @kosiew for trying this, I made a smoke test it worked, checking what is with substrait

comphead · 2026-08-26T20:22:28Z

Also tested with xtask/substrait, seems like working

kosiew

@comphead,

Thanks for the follow-up work here. The previously blocking issues around runtime config handling and continuing past failed combinations look resolved. I have two remaining suggestions around keeping the default and Substrait paths from diverging and adding some end-to-end coverage. Neither is correctness blocker for the current feature.

kosiew · 2026-08-27T09:35:20Z

 }

 #[cfg(feature = "substrait")]
 async fn run_test_file_substrait_round_trip(


Could we avoid duplicating the per-combination dispatch between run_test_file_substrait_round_trip and run_test_file? The two paths differ mainly in the engine they construct, the runner label, and the label passed to count_records, but each independently calls run_each_configuration.

I think it would be safer to collapse this into one function parameterized by a runner factory, leaving a single call site for run_each_configuration. Otherwise a future change could accidentally bypass the matrix loop in the feature-gated Substrait path while leaving the default path correct. That is essentially the structural version of the earlier Substrait bypass we fixed in this PR.

kosiew · 2026-08-27T09:39:05Z

+        assert!(msg.contains("boom"), "got {msg}");
+    }
+
+    #[tokio::test]


It would be useful to add a small checked-in .slt containing a configMatrix directive. Right now the unit tests cover parsing, expansion, failure attribution, and continuing after failures, but no real SLT file exercises the full parse and replay path.

There is a limitation here: a passing matrix .slt cannot prove that every combination actually ran, because all combinations replay the same records and expected output. So I would not rely on this test to protect against skipped combinations. The shared dispatch refactor above is the stronger protection for that.

Still, a small matrix .slt would give useful end-to-end coverage. For Substrait, the CI step would also need to include that file because the current --substrait-round-trip limit.slt filter will not pick it up. The corresponding xtask insta snapshot would need updating as well.

comphead marked this pull request as draft August 19, 2026 17:02

github-actions Bot added the sqllogictest SQL Logic Tests (.slt) label Aug 19, 2026

comphead changed the title ~~Slt matrix~~ Support config matrix for SLT tests Aug 19, 2026

comphead marked this pull request as ready for review August 19, 2026 21:15

comphead requested a review from kosiew August 19, 2026 21:16

kosiew requested changes Aug 20, 2026

View reviewed changes

github-actions Bot added the development-process Related to development process of DataFusion label Aug 24, 2026

comphead added 3 commits August 24, 2026 11:44

feat: support configMatrix for SLT tests

b9c264f

feat: support configMatrix for SLT tests

a5f4a74

feat: support configMatrix for SLT tests

0a4a0ef

comphead force-pushed the slt_matrix branch from 162ed9c to 0a4a0ef Compare August 24, 2026 18:52

comphead requested a review from kosiew August 24, 2026 19:10

kosiew requested changes Aug 25, 2026

View reviewed changes

comphead added 3 commits August 25, 2026 08:06

feat: support configMatrix for SLT tests

49ead27

feat: support configMatrix for SLT tests

8998a28

feat: support configMatrix for SLT tests

4f3739e

github-actions Bot removed the development-process Related to development process of DataFusion label Aug 25, 2026

feat: support configMatrix for SLT tests

f376212

kosiew requested changes Aug 26, 2026

View reviewed changes

feat: support configMatrix for SLT tests

1ad33fc

comphead requested a review from kosiew August 26, 2026 20:22

kosiew approved these changes Aug 27, 2026

View reviewed changes

Uh oh!

Conversation

comphead commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

codecov-commenter commented Aug 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kosiew left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

comphead commented Aug 24, 2026

Uh oh!

kosiew left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

comphead commented Aug 25, 2026

Uh oh!

kosiew left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

comphead commented Aug 26, 2026

Uh oh!

kosiew left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

comphead commented Aug 19, 2026 •

edited

Loading

codecov-commenter commented Aug 19, 2026 •

edited

Loading